Conversation
📝 WalkthroughWalkthroughA plugin-based architecture is introduced for yes/no and option-matching interactions in OVOSSkill. Two engine getter methods with caching load configurable plugins; ask_yesno and ask_selection delegate to these engines with fallback to existing solvers when engines are absent or fail. Changes
Sequence Diagram(s)sequenceDiagram
participant Skill as OVOSSkill
participant Cache as Engine Cache
participant PluginMgr as Plugin Manager
participant Engine as Loaded Engine
participant Solver as YesNoSolver
rect rgb(200, 150, 100, 0.5)
Note over Skill,Solver: ask_yesno() with Plugin Path
Skill->>Cache: Check if engine cached
Cache-->>Skill: Cache miss
Skill->>PluginMgr: Load YesNo plugin by name
PluginMgr-->>Engine: Return engine instance
Engine-->>Cache: Store in cache
Skill->>Engine: Call engine.match(response)
Engine-->>Skill: Return yes/no/None result
alt Engine returns result
Skill-->>Skill: Use matched result
else Engine fails or unavailable
Skill->>Solver: Fall back to YesNoSolver
Solver-->>Skill: Return legacy match result
end
end
rect rgb(100, 150, 200, 0.5)
Note over Skill,Engine: ask_selection() with Plugin Path
Skill->>Cache: Check if engine cached
Cache-->>Skill: Cache miss or default fuzzy
Skill->>PluginMgr: Load OptionMatcher plugin
PluginMgr-->>Engine: Return engine instance
Engine-->>Cache: Store in cache
Skill->>Engine: Call engine.match(response, options)
Engine-->>Skill: Return matched option or None
alt Engine succeeds
Skill-->>Skill: Return matched option
else Engine error
Skill-->>Skill: Log error, return None
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Just the facts, ma'am. Here's your report. 👮♂️I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthA routine checkup to keep the repo running smoothly. 🏥 ✅ All required files present. Latest Version: ✅ 🔍 LintThe data is in, and it's looking interesting! 🧐 ❌ ruff: issues found — see job log 🔨 Build TestsEnsuring the code is correctly packaged and ready. 📦
❌ 3.10: Build OK, install failed Your friendly neighborhood bot 🕷️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ovos_workshop/skills/ovos.py (1)
2007-2057:⚠️ Potential issue | 🟡 MinorThe
min_confparameter is now unused.The
min_confparameter (line 2008) is no longer used in the method body since the legacymatch_onefallback was removed. The parameter signature should either be deprecated or documented as passed to the engine via config.Additionally, when the engine raises an exception (lines 2052-2056), the method returns
Nonewith no fallback matching. This differs from the documentation indocs/skill-interaction.mdline 345 which statesask_yesnofalls back toYesNoSolveron plugin failure, butask_selectiondoes not have equivalent fallback behavior—verify this asymmetry is intentional.Consider documenting or deprecating min_conf
Either remove the parameter with a deprecation warning:
def ask_selection(self, options: List[str], dialog: str = '', data: Optional[dict] = None, min_conf: float = 0.65, numeric: bool = False, num_retries: int = -1): + # Note: min_conf is deprecated and ignored; configure via plugin settingsOr pass it to the engine:
try: - resp = engine.match_option(utterance=resp, options=options, lang=self.lang) + resp = engine.match_option(utterance=resp, options=options, lang=self.lang, min_conf=min_conf)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_workshop/skills/ovos.py` around lines 2007 - 2057, The ask_selection method currently ignores the min_conf parameter and has no fallback when engine.match_option fails; update ask_selection to either (A) deprecate/remove min_conf (emit a DeprecationWarning in ask_selection and update docstring), or (B) pass min_conf to the selection engine by calling engine.match_option(utterance=resp, options=options, lang=self.lang, min_conf=min_conf) so engines can use it; additionally add a safe fallback path in ask_selection in the except block (e.g., call a legacy matcher or a default solver similar to YesNoSolver) so that when _get_selection_engine() exists but engine.match_option raises, the method attempts fallback matching instead of returning None, and ensure logging still records the exception (LOG.error) before falling back.
🧹 Nitpick comments (4)
test/unittests/test_skill_interaction.py (2)
78-112: Missing test for selection engine caching.
TestGetYesnoEngineincludestest_engine_cached_across_calls(lines 69-75), butTestGetSelectionEnginelacks an equivalent test. Consider adding one for consistency:Add caching test for selection engine
def test_engine_cached_across_calls(self): skill = _make_skill() # Uses default fuzzy plugin mock_cls = MagicMock() with patch("ovos_plugin_manager.agents.load_option_matcher_plugin", return_value=mock_cls) as mock_load: skill._get_selection_engine() skill._get_selection_engine() mock_load.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_skill_interaction.py` around lines 78 - 112, Add a caching test to TestGetSelectionEngine to mirror TestGetYesnoEngine: create a new test method (e.g. test_engine_cached_across_calls) that calls skill._get_selection_engine() twice while patching ovos_plugin_manager.agents.load_option_matcher_plugin to return a MagicMock and then assert the plugin load was called only once; reference TestGetSelectionEngine, _get_selection_engine, and load_option_matcher_plugin when locating where to add the test.
218-224: Clarify test intent: no fallback when engine is None.This test (
test_no_engine_no_response_returns_none) patches_get_selection_engineto returnNone, but in production_get_selection_enginewill never returnNoneunless the default fuzzy plugin fails to load. Consider adding a test for when the engine exists but returnsNonefrommatch_option, which is the more common "no match" scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_skill_interaction.py` around lines 218 - 224, The test test_no_engine_no_response_returns_none incorrectly simulates a missing engine by patching _get_selection_engine to return None; instead, add or replace a test that simulates a present engine that yields no match by patching _get_selection_engine to return a mock engine whose match_option method returns None, then call ask_selection(options, numeric=True) and assert it returns None; reference the test name, _get_selection_engine, match_option, and ask_selection so the change targets the selection flow where the engine exists but finds no match.docs/skill-interaction.md (1)
46-49: Add language specifiers to fenced code blocks.Several code blocks for dialog files lack language specifiers. While these are plain text, adding a specifier improves rendering and satisfies linters.
Add language specifiers
-``` +```text Are you sure you want to delete this?Apply similarly to lines 58, 138, and 266.
Also applies to: 58-60, 138-141, 266-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/skill-interaction.md` around lines 46 - 49, Several fenced code blocks containing dialog snippets (e.g., the blocks with text "Are you sure you want to delete this?", "Do you really want to delete it?", and the other plain-text dialog blocks around the same sections) lack language specifiers; update each triple-backtick fence to include a language tag (use "text") so the blocks become ```text ... ```; apply this change to the code fences containing the shown lines and the similar blocks at the other locations referenced in the comment.ovos_workshop/skills/ovos.py (1)
1964-1982: Default plugin always triggers load attempt.Unlike
_get_yesno_engine, this method defaults to"ovos-option-matcher-fuzzy-plugin"when no config is set (line 1972). This means every call will attempt to load a plugin even if the user hasn't configured one. If the default plugin isn't installed, this will log an error on everyask_selectioncall.Consider whether this is intentional—currently
pyproject.tomladds the fuzzy plugin as a dependency, so it should always be available. However, if someone removes it, the error will be noisy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_workshop/skills/ovos.py` around lines 1964 - 1982, The method _get_selection_engine currently defaults plugin_name to "ovos-option-matcher-fuzzy-plugin" causing a load attempt on every call; change it to mirror _get_yesno_engine by not defaulting to a plugin: derive plugin_name only from self.settings and self.config_core (no hardcoded default), compute cache_key the same, and only call load_option_matcher_plugin(plugin_name) and instantiate when plugin_name is truthy; if no plugin_name set, set the cache attribute to None so future calls are cached and avoid repeated error logs from load_option_matcher_plugin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/skill-interaction.md`:
- Around line 104-114: The docs claim the min_conf parameter is applied but the
implementation no longer uses it directly; update the documentation for the
options parameter table to state that min_conf is currently unused in the
high-level API (or is forwarded only to the OptionMatcherEngine configuration if
applicable) or mark min_conf as deprecated. Specifically, change the `min_conf`
row to indicate its current behavior (e.g., "currently unused in the high-level
selection flow; only used when configuring OptionMatcherEngine" or "deprecated")
and mention the OptionMatcherEngine symbol so readers can find where to
set/override this value.
- Around line 340-349: The doc claims ask_yesno falls back to YesNoSolver if the
plugin raises, but the code calls engine.yes_or_no() directly; wrap the
engine.yes_or_no() call inside a try/except in the ask_yesno implementation so
any exception from the engine is caught, log the exception, and then invoke the
fallback YesNoSolver path (same return behavior as when plugin fails to load) to
match the documentation; ensure you reference engine.yes_or_no(), ask_yesno, and
YesNoSolver when making the change so the fallback is triggered on runtime
errors.
In `@test/unittests/test_skill_interaction.py`:
- Line 1: Update the copyright header string from "2024" to "2026" in the file
(the top-line copyright comment), ensuring the header now reads "Copyright 2026
Mycroft AI Inc." so the year is current; keep formatting identical aside from
the year change.
---
Outside diff comments:
In `@ovos_workshop/skills/ovos.py`:
- Around line 2007-2057: The ask_selection method currently ignores the min_conf
parameter and has no fallback when engine.match_option fails; update
ask_selection to either (A) deprecate/remove min_conf (emit a DeprecationWarning
in ask_selection and update docstring), or (B) pass min_conf to the selection
engine by calling engine.match_option(utterance=resp, options=options,
lang=self.lang, min_conf=min_conf) so engines can use it; additionally add a
safe fallback path in ask_selection in the except block (e.g., call a legacy
matcher or a default solver similar to YesNoSolver) so that when
_get_selection_engine() exists but engine.match_option raises, the method
attempts fallback matching instead of returning None, and ensure logging still
records the exception (LOG.error) before falling back.
---
Nitpick comments:
In `@docs/skill-interaction.md`:
- Around line 46-49: Several fenced code blocks containing dialog snippets
(e.g., the blocks with text "Are you sure you want to delete this?", "Do you
really want to delete it?", and the other plain-text dialog blocks around the
same sections) lack language specifiers; update each triple-backtick fence to
include a language tag (use "text") so the blocks become ```text ... ```; apply
this change to the code fences containing the shown lines and the similar blocks
at the other locations referenced in the comment.
In `@ovos_workshop/skills/ovos.py`:
- Around line 1964-1982: The method _get_selection_engine currently defaults
plugin_name to "ovos-option-matcher-fuzzy-plugin" causing a load attempt on
every call; change it to mirror _get_yesno_engine by not defaulting to a plugin:
derive plugin_name only from self.settings and self.config_core (no hardcoded
default), compute cache_key the same, and only call
load_option_matcher_plugin(plugin_name) and instantiate when plugin_name is
truthy; if no plugin_name set, set the cache attribute to None so future calls
are cached and avoid repeated error logs from load_option_matcher_plugin.
In `@test/unittests/test_skill_interaction.py`:
- Around line 78-112: Add a caching test to TestGetSelectionEngine to mirror
TestGetYesnoEngine: create a new test method (e.g.
test_engine_cached_across_calls) that calls skill._get_selection_engine() twice
while patching ovos_plugin_manager.agents.load_option_matcher_plugin to return a
MagicMock and then assert the plugin load was called only once; reference
TestGetSelectionEngine, _get_selection_engine, and load_option_matcher_plugin
when locating where to add the test.
- Around line 218-224: The test test_no_engine_no_response_returns_none
incorrectly simulates a missing engine by patching _get_selection_engine to
return None; instead, add or replace a test that simulates a present engine that
yields no match by patching _get_selection_engine to return a mock engine whose
match_option method returns None, then call ask_selection(options, numeric=True)
and assert it returns None; reference the test name, _get_selection_engine,
match_option, and ask_selection so the change targets the selection flow where
the engine exists but finds no match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbbfb193-fd76-43c4-860b-7d60806f6338
📒 Files selected for processing (4)
docs/skill-interaction.mdovos_workshop/skills/ovos.pypyproject.tomltest/unittests/test_skill_interaction.py
| ### Parameters | ||
|
|
||
| | Parameter | Type | Default | Description | | ||
| |-----------|------|---------|-------------| | ||
| | `options` | `List[str]` | — | The predefined options to offer. | | ||
| | `dialog` | `str` | `''` | Dialog ID or literal string spoken **after** the options list. | | ||
| | `data` | `dict \| None` | `None` | Template variables for the dialog string. | | ||
| | `min_conf` | `float` | `0.65` | Minimum fuzzy-match confidence for the default plugin. Passed to the `OptionMatcherEngine` config if no plugin-level config overrides it. | | ||
| | `numeric` | `bool` | `False` | If `True`, speaks each option prefixed with its number ("one, pizza; two, pasta; …"). If `False`, speaks them as a joined list ("pizza, pasta, or salad?"). | | ||
| | `num_retries` | `int` | `-1` | How many times to re-prompt on no response. `-1` means use the system default. | | ||
|
|
There was a problem hiding this comment.
Document that min_conf parameter is currently unused.
The min_conf parameter is documented (line 111) but is not used in the current implementation since the legacy match_one was removed. Update the documentation to clarify this is passed to the engine configuration or mark it as deprecated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/skill-interaction.md` around lines 104 - 114, The docs claim the
min_conf parameter is applied but the implementation no longer uses it directly;
update the documentation for the options parameter table to state that min_conf
is currently unused in the high-level API (or is forwarded only to the
OptionMatcherEngine configuration if applicable) or mark min_conf as deprecated.
Specifically, change the `min_conf` row to indicate its current behavior (e.g.,
"currently unused in the high-level selection flow; only used when configuring
OptionMatcherEngine" or "deprecated") and mention the OptionMatcherEngine symbol
so readers can find where to set/override this value.
| | Scenario | ask_yesno result | ask_selection result | | ||
| |----------|-----------------|---------------------| | ||
| | User says nothing (timeout) | `None` | `None` | | ||
| | User response unclassifiable | raw transcript string | `None` | | ||
| | Plugin fails to load | falls back to `YesNoSolver` | `None` | | ||
| | Plugin raises at runtime | falls back to `YesNoSolver` | `None` | | ||
| | No plugin configured | `YesNoSolver` used | default fuzzy plugin used | | ||
|
|
||
| `ask_selection` is intentionally strict: any failure returns `None` rather than guessing. Always handle the `None` case in your skill. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ask_yesno exception handling
rg -n -A 10 "def ask_yesno" ovos_workshop/skills/ovos.py | head -30Repository: OpenVoiceOS/ovos-workshop
Length of output: 693
🏁 Script executed:
#!/bin/bash
# Get complete ask_yesno method implementation
rg -n -A 25 "def ask_yesno" ovos_workshop/skills/ovos.pyRepository: OpenVoiceOS/ovos-workshop
Length of output: 1405
🏁 Script executed:
#!/bin/bash
# Check the documentation file at the specified lines
sed -n '335,355p' docs/skill-interaction.mdRepository: OpenVoiceOS/ovos-workshop
Length of output: 1030
🏁 Script executed:
#!/bin/bash
# Search for any try/except blocks in ask_yesno or related plugin handling
rg -n -B 5 -A 10 "yes_or_no\|YesNoSolver" ovos_workshop/skills/ovos.pyRepository: OpenVoiceOS/ovos-workshop
Length of output: 51
Documentation states fallback behavior that doesn't match code.
The documentation at lines 340-349 claims "Plugin raises at runtime → falls back to YesNoSolver" for ask_yesno, but the actual implementation at ovos_workshop/skills/ovos.py lines 1996-1997 calls engine.yes_or_no() without any try/except wrapper. If the engine raises an exception, it will propagate rather than falling back to YesNoSolver.
Update the code to add exception handling around engine.yes_or_no() to match the documented behavior, or update the documentation to reflect that exceptions propagate uncaught.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/skill-interaction.md` around lines 340 - 349, The doc claims ask_yesno
falls back to YesNoSolver if the plugin raises, but the code calls
engine.yes_or_no() directly; wrap the engine.yes_or_no() call inside a
try/except in the ask_yesno implementation so any exception from the engine is
caught, log the exception, and then invoke the fallback YesNoSolver path (same
return behavior as when plugin fails to load) to match the documentation; ensure
you reference engine.yes_or_no(), ask_yesno, and YesNoSolver when making the
change so the fallback is triggered on runtime errors.
| @@ -0,0 +1,264 @@ | |||
| # Copyright 2024 Mycroft AI Inc. | |||
There was a problem hiding this comment.
Copyright year appears outdated.
The copyright header says "2024" but the current year is 2026.
Fix copyright year
-# Copyright 2024 Mycroft AI Inc.
+# Copyright 2026 OpenVoiceOS📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Copyright 2024 Mycroft AI Inc. | |
| # Copyright 2026 OpenVoiceOS |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_skill_interaction.py` at line 1, Update the copyright
header string from "2024" to "2026" in the file (the top-line copyright
comment), ensuring the header now reads "Copyright 2026 Mycroft AI Inc." so the
year is current; keep formatting identical aside from the year change.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Chores